Skip to content

Update resource metadata if new config is given#4012

Closed
kayue wants to merge 4 commits into
api-platform:mainfrom
kayue:patch-2
Closed

Update resource metadata if new config is given#4012
kayue wants to merge 4 commits into
api-platform:mainfrom
kayue:patch-2

Conversation

@kayue

@kayue kayue commented Feb 2, 2021

Copy link
Copy Markdown

If a resource is being defined twice, allow the later one to override the previous one.

Q A
Bug fix? no
New feature? yes
BC breaks? yes
Deprecations? no
Tickets
License MIT
Doc PR

If a resource is being defined twice, allow the later one to override the previous one.

This is useful when resources is predefined by a 3rd party library, eg Sylius.

If a resource is being defined twice, allow the later one to override the previous one.
Comment thread src/Metadata/Resource/Factory/ExtractorResourceMetadataFactory.php Outdated
Comment thread src/Metadata/Resource/Factory/ExtractorResourceMetadataFactory.php Outdated
@dunglas

dunglas commented Feb 2, 2021

Copy link
Copy Markdown
Member

API Platform has always worked like that (replacing and not merging), and it's on purpose for reasons explained by @alanpoulain. We should find a way to allow an easier customization of the configuration provided by third parties such as Sylius, but we cannot change this behavior like that in a minor version, this could break and even introduce major security issues in existing projets (and I'm not sure that merging is desirable in the context of API Platform, but we can discuss this for 3.0).

We have the same "issue" for defaults by the way (merging could be convenient and maybe expected, but it's inconsistent with the other parts of the framework).

@kayue

kayue commented Feb 3, 2021

Copy link
Copy Markdown
Author

@dunglas @alanpoulain Understood. If merging is not acceptable, how about allow latter config to override previous one completely? Does the same security concern apply?

@dunglas

dunglas commented Feb 11, 2021

Copy link
Copy Markdown
Member

This looks like a reasonable approach to me!

@kayue

kayue commented Feb 14, 2021

Copy link
Copy Markdown
Author

@dunglas @alanpoulain I have added test cases to cover the override function. Now it works like the following:

Parent:

resources:
    'ApiPlatform\Core\Tests\Fixtures\TestBundle\Entity\FileConfigDummy':
        description: 'Dummy resource'
        itemOperations:
            my_op_name:
                method: 'GET'
            my_other_op_name:
                method: 'POST'
        collectionOperations:
            my_collection_op:
                method: 'POST'
                path: 'the/collection/path'
                status: '201'

Child:

resources:
    'ApiPlatform\Core\Tests\Fixtures\TestBundle\Entity\FileConfigDummy':
        description: New description # Override a text node
        itemOperations:
            my_op_name:
                method: 'GET'
                path: 'the/path' # Add a new node
        collectionOperations:
            my_collection_op: # Removed a "path" node
                method: 'POST'
                status: '200' # Override a node

Result:

resources:
    'ApiPlatform\Core\Tests\Fixtures\TestBundle\Entity\FileConfigDummy':
        description: 'New description' # Updated
        itemOperations:
            my_op_name:  # New node added.
                method: 'GET'
                path: 'the/path'
            my_other_op_name: # Remain untouched.
                method: 'POST'
        collectionOperations:
            my_collection_op: # The node "path" is now removed.
                method: 'POST'
                status: '200' # The node "status" is updated.

@alanpoulain

Copy link
Copy Markdown
Member

I'm not sure we want to merge the operations like this either.
And it's a BC anyway.
Why not doing this is in Sylius, since most use cases are there?

@kayue

kayue commented Feb 15, 2021

Copy link
Copy Markdown
Author

@alanpoulain I continued the work because @dunglas thought it is a reasonable approach.

ApiPlatform aim to be the backbone of many projects, therefore I think it is a flexibility ApiPlatform should allow. Extendability is one of the best of Symfony ecosystem offer, and we should do the same here.

Regarding your suggestion on decorate the ExtractorResourceMetadataFactory service, it would require copy and paste a large trunk of code, therefore I would like to avoid it in my project if possible.

I think we just need to decide whether this is an expected behaviour of ApiPlatform. In my opinion config overwrite is a common feature of Symfony bundles, I don't see why ApiPlatform is different.

I understand this is a big BC break and I can understand this feature has to wait until the next major version. I just think this can be a useful feature for many projects out there.

To me I think the least ApiPlatform can do is to throw a warning when it decided to ignore developer's configuration.

@dunglas

dunglas commented Feb 15, 2021

Copy link
Copy Markdown
Member

Maybe can we just provide a new attribute merge=true to make this behavior opt-in? As it can be set in defaults, we have best of both worlds. If it's proved convenient we could even enable this attribute by default in API Platform 3. WDYT?

@alanpoulain

Copy link
Copy Markdown
Member

Yes if we want this feature it should be configurable with an attribute to prevent BC (and to not wait for the next major).
Not sure about making it the default but we'll see!

@kayue

kayue commented Feb 15, 2021

Copy link
Copy Markdown
Author

@dunglas @alanpoulain Should I can do that. Can you be a bit more specific on how you see this attribute works? Something like this?

resources:
    ApiPlatform\Core\Tests\Fixtures\TestBundle\Entity\FileConfigDummy:
        description: 'Dummy resource'
        attributes:
            merge: true

I assume this is an attribute defined in the child resource to allow developer to force merging?

(Where can I found the docs for defaults options? Couldn't found it on the official site.)

@alanpoulain

alanpoulain commented Feb 15, 2021

Copy link
Copy Markdown
Member

Should be OK in the attributes array indeed.
Having it in the child resource seems right to me.
I'm not sure there is a doc (it would be nice to have one). PR is here: #3151

@andriusvo

andriusvo commented Apr 8, 2021

Copy link
Copy Markdown

@kayue Corrections should be done. I can give an example from real world with Sylius. F.e. we have itemOperation defined:

            <itemOperation name="shop_apply_coupon">
                <attribute name="method">PATCH</attribute>
                <attribute name="path">/shop/orders/{id}/apply-coupon</attribute>
                <attribute name="messenger">input</attribute>
                <attribute name="input">Sylius\Bundle\ApiBundle\Command\Cart\ApplyCouponToCart</attribute>
                <attribute name="denormalization_context">
                    <attribute name="groups">shop:cart:apply_coupon</attribute>
                </attribute>
                <attribute name="openapi_context">
                    <attribute name="summary">Applies coupon to cart</attribute>
                </attribute>
            </itemOperation>

And I want to add attribute validation_groups to this itemOperation. So, I expect to write the following code:

resources:
    App\Entity\Order\Order:
        itemOperations:
            shop_apply_coupon:
                validation_groups: [sylius, some_validation_group]

With function array_merge, only validation_groups attribute appear, and other attributes from $parentPropertyMetadata dissapear. In this case, maybe it's better to use function array_replace_recursive or array_merge_recursive?

@kayue

kayue commented Apr 8, 2021

Copy link
Copy Markdown
Author

@andriusvo I don't know does Api Platform want this merging feature at all.

@alanpoulain

Copy link
Copy Markdown
Member

If this is opt-in, yes!

@antonioperic

Copy link
Copy Markdown

It is not promoting our blog,
but here is solution how we did it in company https://locastic.com/blog/clean-api-configuration-in-sylius/

@stale

stale Bot commented Nov 4, 2022

Copy link
Copy Markdown

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale Bot added the wontfix label Nov 4, 2022
@stale

stale Bot commented Jan 3, 2023

Copy link
Copy Markdown

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale Bot added the stale label Jan 3, 2023
@stale stale Bot closed this Jan 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants